Skip to content

fix(idempotency): @IdempotencyStore.wrap supports arg-projected methods#567

Merged
bokelley merged 2 commits intomainfrom
bokelley/issue-559-idempotency-arg-projection
May 4, 2026
Merged

fix(idempotency): @IdempotencyStore.wrap supports arg-projected methods#567
bokelley merged 2 commits intomainfrom
bokelley/issue-559-idempotency-arg-projection

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

@bokelley bokelley commented May 4, 2026

Closes #559.

The bug

`@IdempotencyStore.wrap` had signature `(handler_self, params, context=None, *args, **kwargs)`. The framework's arg-projector path in `dispatch.py:1081` calls methods as `method(**arg_projector, ctx=ctx)` — for `update_media_buy`, that's `method(self, media_buy_id=..., patch=..., ctx=...)`. Python can't bind `params` (positional, no default) → `TypeError: missing 1 required positional argument: 'params'` before the wrap body runs.

Salesagent's kill-nginx spike shipped a workaround dropping `@_IDEMPOTENCY.wrap` from `update_media_buy` entirely. Buyer retries re-execute side effects.

Fix

Wrap signature is now `async def _wrapped(*args, **kwargs)`. A new `_resolve_call_args` helper detects which of three calling conventions the framework dispatched with:

  1. Positional: `(self, params, ctx)` — default for non-projected tools.
  2. Keyword: `(self, params=..., context=...)`.
  3. Arg-projected: `(self, **arg_projector_kwargs, ctx=...)` — searches kwargs for a Pydantic model (the framework's contract for the request model, e.g. `patch` on `update_media_buy`) and uses it as the hash source. Falls back to the kwargs dict (excluding `ctx` / `context`) when no Pydantic model is present.

The inner handler is invoked via `*args/**kwargs` verbatim. The wrap is signature-transparent — no matter which convention the framework used, the inner handler sees its expected arguments unchanged.

Why a wrap-side fix (not dispatch-side)?

Three reasons:

  • Dispatch is generic and shouldn't know about `@wrap` — the abstraction was already correct.
  • Adopters who call `@store.wrap` outside the framework dispatch (in custom transports, in tests) get the fix automatically without retrofitting their callers.
  • The `_resolve_call_args` heuristic uses a public Pydantic contract (`hasattr(value, "model_dump")`), not framework-internal knowledge, so future arg-projector additions work without further wrap changes.

Edge cases handled

  • `update_media_buy` (`arg_projector={"media_buy_id": id, "patch": }`): finds `patch` (the only Pydantic kwarg), extracts `idempotency_key` from it. Cache hit + replay + conflict all work. ✅
  • `sync_audiences` style (`arg_projector={"audiences": [...]}`): no Pydantic model, no top-level `idempotency_key` → wrap finds no key, runs handler without dedup. Same fall-through as before for missing-key — not a regression. Adopters who want idempotency on this shape should project the request model directly. ✅
  • Top-level `idempotency_key` in arg_projector kwargs: fallback path treats kwargs (minus `ctx`/`context`) as the hash source. Dedup works. ✅
  • Cross-principal isolation: arg-projected calls use `ctx=` (not `context=`). The resolver checks both. Same key from different principals → different cache scope (no cross-principal replay). ✅
  • Conflict on same key + different patch: `IdempotencyConflictError` raised, parity with positional path. ✅

Tests

6 new in `TestWrapArgProjectionCalling`:

  • `update_media_buy` shape: cache hit, replay, conflict
  • `sync_audiences` style: no-key fall-through
  • Top-level `idempotency_key` fallback
  • `ctx=` kwarg scope-key extraction (cross-principal isolation)
  • Keyword convention (`params=`, `context=`)

154 tests green across the broader idempotency surface (validate-wiring, PG backend, MCP/A2A integration). Lint clean.

Test plan

  • `pytest tests/test_server_idempotency.py` — 55 passed (49 existing + 6 new)
  • `pytest tests/test_idempotency.py tests/test_validate_idempotency_wiring.py tests/test_pg_idempotency_backend.py` — 154 total green
  • `ruff check` — clean
  • Salesagent re-applies `@_IDEMPOTENCY.wrap` to `update_media_buy` and verifies storyboard runner passes

🤖 Generated with Claude Code

bokelley and others added 2 commits May 4, 2026 07:10
…ds (closes #559)

Wrap was hard-coded for the (self, params, ctx) calling convention,
so framework-dispatched arg-projected tools like update_media_buy —
called as method(self, media_buy_id=..., patch=..., ctx=ctx) by
dispatch.py's arg_projector path — raised TypeError before the wrap
body ran. Salesagent shipped a workaround dropping @Wrap from
update_media_buy entirely, losing idempotency on retries.

Wrap now resolves the calling convention via _resolve_call_args()
and supports three shapes:

1. Positional (self, params, ctx) — original.
2. Keyword (self, params=..., context=...).
3. Arg-projected (self, **arg_projector_kwargs, ctx=...) — searches
   kwargs for a Pydantic model (the framework's contract for the
   request model, e.g. patch on update_media_buy) to pull
   idempotency_key from. Falls back to the kwargs dict (excluding
   ctx/context) when no Pydantic model is present, so projections
   exposing idempotency_key at the top level still work.

Tests: 6 new in TestWrapArgProjectionCalling, 154 across the broader
idempotency surface green. Lint clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ydantic, BaseModel strictness)

- _resolve_call_args: 'kwargs.get(context) or kwargs.get(ctx)' silently
  fell through to ctx when context was explicitly None. Switched to
  'in'-based check so explicit None wins.
- Multi-Pydantic-kwarg: prefer 'params' / 'request' / 'patch' by name
  before falling back to first-by-iteration. Eliminates dict-order
  fragility when a tool has two model kwargs.
- isinstance(BaseModel) instead of hasattr(model_dump). A duck-typed
  object with model_dump no longer accidentally matches.

4 new tests cover: explicit None-context wins, multi-Pydantic
preference order, no-preferred-name first-by-iteration fallback,
duck-typed non-Pydantic exclusion. 59 tests green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley
Copy link
Copy Markdown
Contributor Author

bokelley commented May 4, 2026

Updated with expert-review fixes (code-reviewer + ad-tech-protocol-expert):

Code-reviewer must-fix:

  • Falsy-context fall-through: kwargs.get('context') or kwargs.get('ctx') would silently use ctx if context was explicitly None. Switched to in-based check so explicit None wins.
  • Multi-Pydantic-kwarg ordering: prefer kwargs literally named params / request / patch before falling back to first-by-iteration. Eliminates dict-order fragility when a tool has two Pydantic models in kwargs.
  • Tightened detection from hasattr(model_dump) to isinstance(BaseModel). Duck-typed objects with a model_dump method no longer accidentally match.

Tests added: 4 new in TestWrapResolveCallArgsEdgeCases covering: explicit None context wins, multi-Pydantic preference order, no-preferred-name fallback, duck-typed exclusion.

Protocol-expert open question (#1) — should target IDs (e.g. media_buy_id for update_media_buy) be part of the cache scope so a buggy buyer reusing a key across two different targets gets a CONFLICT rather than a wrong-target replay? Currently scope is principal+tenant only, body hash includes patch (which doesn't carry media_buy_id). I think this is consistent with AdCP #2315's principal-scope model — the spec treats key reuse across targets as buyer error, not server-detected conflict — but worth verifying. Left as a follow-up; not changing scope semantics in this PR.

CI running on the new commit. 59 tests green locally; lint clean.

@bokelley
Copy link
Copy Markdown
Contributor Author

bokelley commented May 4, 2026

Fixes look solid — the in-based context check and isinstance(BaseModel) tightening are exactly the right calls; the duck-typed exclusion in particular was a latent footgun.

On the target-ID scoping question: your read of AdCP #2315 is correct. The principal-scope model places the key-uniqueness burden on the buyer, and key reuse across different targets is buyer error per spec. That said, media_buy_id is a routing discriminant (more like a URL path parameter than a body field), and HTTP-level idempotency convention generally includes the resource identity in scope — so there's a defensible case for optional target-ID-aware scoping as a follow-up. Worth its own issue to track; agreed this is not a blocker for this PR.


Generated by Claude Code

@bokelley
Copy link
Copy Markdown
Contributor Author

bokelley commented May 4, 2026

Thanks for the thorough review, @bokelley. Glad the in-based context check and isinstance(BaseModel) tightening land well.

Agreed on the target-ID scoping follow-up — the spec-alignment reasoning (buyer owns key uniqueness) is sound for this PR, and the HTTP-level resource-identity angle is worth its own issue to track cleanly. Won't create it here; leaving that to the team.

Ready for merge when Salesagent's re-application of @_IDEMPOTENCY.wrap to update_media_buy is confirmed green.


Generated by Claude Code

@bokelley bokelley merged commit 1abdae6 into main May 4, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@IdempotencyStore.wrap incompatible with arg-projected methods (update_media_buy)

1 participant